Skip to content

Conversation

@cwfitzgerald
Copy link

@cwfitzgerald cwfitzgerald commented Nov 17, 2019

When a timer is made within a timer handler, it is possible to end up with an underflow with the duration-since-start calculation. Code demonstrating the bug is as follows:

#include <timer.h>

auto timer = timer_create_default();

bool longTask() {
  Serial.println("Starting!");
  delay(50); // simulate really computationally intensive work
  Serial.println("Finally finished!");
  timer.in(1000, [](void*) { 
    Serial.println("An entire second after finishing");
    return false;
  }); // This task will never wait.
  return true;
}

void setup() {
  Serial.begin(9600);

  timer.every(2000, longTask);
}

void loop() {
  timer.tick();
}

This causes the 1000ms delay to happen instantly instead of waiting. The serial output on my Arduino Uno is:

01:19:24.785 -> Starting!
01:19:24.851 -> Finally finished!
01:19:24.851 -> An entire second after finishing

After applying my fix, it is as follows:

01:20:54.249 -> Starting!
01:20:54.315 -> Finally finished!
01:20:55.311 -> An entire second after finishing

This is the correct behavior.

There are two ways of solving this issue, first would be to use signed longs everywhere that duration is used, but I have concerns about overflow on long-running projects, the second is to just ensure the duration isn't negative with a check. This is more computationally expensive, but I believe it should be negligible. A task can't wait for a negative time, so this case will only happen when this bug is showing its face.

@contrem
Copy link
Owner

contrem commented Nov 18, 2019

Thanks for contributing to arduino-timer.

However, this change breaks time overflow/rollover handling. One way to accomplish what you want in your example is to create a separate timer instance for your sub events:

#include <timer.h>

auto timer = timer_create_default();
auto sub_timer = timer_create_default();

bool longTask() {
  Serial.println("Starting!");
  delay(50); // simulate really computationally intensive work
  Serial.println("Finally finished!");
  sub_timer.in(1000, [](void*) { 
    Serial.println("An entire second after finishing");
    return false;
  }); // This task will never wait.
  return true;
}

void setup() {
  Serial.begin(9600);

  timer.every(2000, longTask);
}

void loop() {
  timer.tick();
  sub_timer.tick();
}

I will update the documentation to clarify that adding timer events within another event is not supported.

@contrem
Copy link
Owner

contrem commented Nov 18, 2019

Please try the code at mc/fix-task-within-handler. It should support adding a task within a timer handler.

Thanks for helping improve the arduino-timer library.

@contrem
Copy link
Owner

contrem commented Nov 22, 2019

The fix for this issue is now in master version 1.0.1. Closing.

@contrem contrem closed this Nov 22, 2019
@cwfitzgerald cwfitzgerald deleted the fix-underflow branch November 22, 2019 04:47
@cwfitzgerald
Copy link
Author

Wonderful! Thanks for the fixes, that works perfectly!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants